Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(magentic-one): Enhance error handling in orchestrate_step to manage invalid ledger formats #4845

Merged
merged 5 commits into from
Jan 8, 2025
Merged

fix(magentic-one): Enhance error handling in orchestrate_step to manage invalid ledger formats #4845

merged 5 commits into from
Jan 8, 2025

Conversation

r-bit-rry
Copy link
Contributor

lets

Why are these changes needed?

fix the selected code, progress_ledger[key] might not be a string, it can also be Boolean or integer or whatever, in that case we will get a TypeError and except out of the loop, instead it should be handled as part of the handling of key_error in the selected code

Related issue number

Checks

@jackgerrits jackgerrits requested a review from afourney December 30, 2024 20:11
@afourney
Copy link
Member

Thanks. This looks good to me and catches a reasonable error case.

We weren't seeing this often on our end. Would you mind sharing which version of which model you are using?

(In any case, this is a good error to catch)

@r-bit-rry
Copy link
Contributor Author

I'm fiddling with local models, mostly Llama3.2-vision served through ollama.
Since both ollama and azure/openai API support structured output, and receiving output schema. maybe we should move to pydantic BaseModel and declare the ledge and other classes/object that we would like to receive in a structured way, can maybe even save tokens in the prompts and retry attempts:
https://ollama.com/blog/structured-outputs
https://learn.microsoft.com/en-us/azure/ai-services/openai/how-to/structured-outputs?tabs=python-secure

@jackgerrits
Copy link
Member

@r-bit-rry looks like the formatting just needs to be fixed and then we can get this merged.

That can be fixed with poe fmt, please see instructions here

@jackgerrits jackgerrits enabled auto-merge (squash) January 7, 2025 21:47
@codecov Codecov
Copy link

codecov bot commented Jan 8, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 68.12%. Comparing base (52c2a70) to head (d38726e).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...p_chat/_magentic_one/_magentic_one_orchestrator.py 0.00% 3 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #4845      +/-   ##
==========================================
- Coverage   68.14%   68.12%   -0.03%     
==========================================
  Files         161      161              
  Lines       10185    10187       +2     
==========================================
- Hits         6941     6940       -1     
- Misses       3244     3247       +3
Flag Coverage Δ
unittests 68.12% <0.00%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jackgerrits jackgerrits merged commit 00b06ab into microsoft:main Jan 8, 2025
57 of 58 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@afourney afourney afourney approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@r-bit-rry @afourney @jackgerrits @ekzhu